fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

jungle-amazon · 2023-01-25T00:49:08Z

Replace stage association error with warning
Deprecate stackId in TargetApplication options
Provide a default dynamic stack name for CreateTargetApplication stack with a reference to the application name
Provide a default dynamic stack name for ExistingTargetApplication stack with a reference to the application ID

This fixes: 23861

Note: With this change to stackName, you may run into the following error during deployment if you have been using the default stack id and name by not explicitly setting them.

Resource handler returned message: "You already own an application 'MyApplicationName' (Service: ServiceCatalogAppRegistry, Status Code: 409, Request ID: xxxx)" (RequestToken: yyyy, HandlerErrorCode: InvalidRequest)

To address this error, explicitly set the stackName value to the name of your existing stack. For example:

const associatedApp = new ApplicationAssociator(app, 'MyApplicationAssociator', {
  applications: [ TargetApplication.createApplicationStack({
    applicationName: 'MyApplicationName',
    stackName: 'ApplicationAssociatorStack', // add your existing stack name here
    ...

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…t defects - Replace stage association error with warning - Replace the hardcoded stack ID for CreateTargetApplication with a reference to the application name - Replace the hardcoded stack ID for ExistingTargetApplication with a reference to the application ID

gitpod-io · 2023-01-25T00:49:14Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

rix0rrr

Since renaming a stack is not possible, what will happen in practice is that the old Application will be deleted and a new Application will be created (or actually, since CDK doesn't automatically clean up old stacks, a new application will be created, full stop).

If the new application name is the same as the old application name, the creation will fail.

This means that users currently using this library, whenever they upgrade, will be stuck with an undeployable code base, that needs manual work before they can deploy again. They will need to get rid of the applicationassociator in the old code base, deploy again to unregister all stacks, then upgrade the library and then use the new application associator.

Are you willing to put your users through that? If not, you probably need some mechanism for backwards compatibility so people with existing code bases won't end up stuck.

rix0rrr · 2023-02-01T09:14:45Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts

@@ -90,7 +91,7 @@ class CreateTargetApplication extends TargetApplication {
    super();
  }
  public bind(scope: Construct): BindTargetApplicationResult {
-    const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';
+    const stackId = this.applicationOptions.stackId ?? `CreateTargetApplication${this.applicationOptions.applicationName}`;


After thinking about it a little, I think this is the wrong way to go about it. A Stack has two identifiers:

id, used as an ID in the construct tree (needs to be unique within the scope in the construct tree)

stackName, the name of the CloudFormation Stack being deployed (needs to be unique within the account+region it is deployed to, defaults to id if not supplied).

I think what is happening here is that you are trying to change stackName by changing id, is that right?

If that is the case, doesn't it make more sense to leave id where it is, and explicitly set stackName to the value you're trying to achieve?

Also, CreateXxx is not a great name for a resource (Create is a Verb, but a resource is a Noun). Maybe make it something Application-MyGreatApplication-Stack ?

Yes you are correct, we are trying to change stackName by changing the id. Your proposed approach of setting the stackName makes more sense and I have updated the commit with this change instead.

I also updated the stackName format as per your suggestion.

For your first concern about "renaming" the stack, it is true that if the user wants to continue to rely on the default stack name and keep the same application name, the user will see a conflict because the new stack with the updated name will attempt to create an app that already exists.

I think a workaround for the user would be to specify the previous default stack name ApplicationAssociatorStack as a stackName option property to keep using the old stack name.

I think a workaround for the user would be to specify the previous default stack name ApplicationAssociatorStack as a stackName option property to keep using the old stack name.

👍 Sounds good.

The next question is: how will users that get affected by this know that they have to do that?

Here is my suggestion: they will probably read the change log and find this PR. Please edit the description of this PR and add a section that says "if you run into the following error (...error message here...) make sure to set stackName to the name of your stack. For example:

new ApplicationAssociator(..., { stackName: 'whatever-theold-<your application name here>-stackname-was'

"

Something like that.

That makes a lot of sense, I've added this section in the PR description.

… or id for stack name

Pull request has been modified.

rix0rrr · 2023-02-07T10:05:04Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts

@@ -117,7 +119,11 @@ class ExistingTargetApplication extends TargetApplication {
    super();
  }
  public bind(scope: Construct): BindTargetApplicationResult {
+    const arnComponents = cdk.Arn.split(this.applicationOptions.applicationArnValue, cdk.ArnFormat.SLASH_RESOURCE_SLASH_RESOURCE_NAME);
+    const applicationId = arnComponents.resourceName;
    const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';


Since it looks like stackId originally existed to change the stack name (but was the wrong mechanism for that), can we also deprecate stackId?

Add:

* @deprecated Use `stackName` instead to control the name of the stack

To its docstring?

Yes this will be more helpful to the user. I'll add a deprecated statement to the docstring in the next commit.

rix0rrr · 2023-02-07T10:07:21Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts

@@ -90,7 +91,7 @@ class CreateTargetApplication extends TargetApplication {
    super();
  }
  public bind(scope: Construct): BindTargetApplicationResult {
-    const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';
+    const stackId = this.applicationOptions.stackId ?? `CreateTargetApplication${this.applicationOptions.applicationName}`;


I think a workaround for the user would be to specify the previous default stack name ApplicationAssociatorStack as a stackName option property to keep using the old stack name.

👍 Sounds good.

The next question is: how will users that get affected by this know that they have to do that?

Here is my suggestion: they will probably read the change log and find this PR. Please edit the description of this PR and add a section that says "if you run into the following error (...error message here...) make sure to set stackName to the name of your stack. For example:

new ApplicationAssociator(..., { stackName: 'whatever-theold-<your application name here>-stackname-was'

"

Something like that.

…options

Pull request has been modified.

mergify · 2023-02-08T09:08:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-02-08T09:48:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e6fd324
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-02-08T09:49:06Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ack name for Application stack (#24171) * Assign value of `stackName` to `stackId` for Application stack, so the stack id will always be the same as stack name. Users wanting to control stack id can do so via `stackName`. Closes #24160. Background: * Customers wish to control or modify the stack id of the Application stack to follow their project conventions within CDK. * In previous [fix](#23823), we had deprecated the stack id to push users towards using stack name as the main mechanism for stack identification. Note on backward-compatibility: After this change, the `stackId` parameter can no longer be used to control the application stack id. The stack id will always match stack name, and the default stack name if not specified will be `Application-${APPLICATION_IDENTIFIER}-Stack`. `${APPLICATION_IDENTIFIER}` is application name for CreateTargetApplication and application id for ExistingTargetApplication. If you created an application stack prior to aws-cdk [release v2.64.0](https://github.com/aws/aws-cdk/releases/tag/v2.64.0) and did not specify a stack id or name, you may run into the following error during deployment due to the creation attempt of a new stack with the same application: ```log Resource handler returned message: "You already own an application 'MyApplicationName' (Service: ServiceCatalogAppRegistry, Status Code: 409, Request ID: xxxx)" (RequestToken: yyyy, HandlerErrorCode: InvalidRequest) ``` To address this error, you can set the `stackName` value to match your existing stack. For example: ```typescript const associatedApp = new ApplicationAssociator(app, 'MyApplicationAssociator', { applications: [ TargetApplication.createApplicationStack({ applicationName: 'MyApplicationName', stackName: 'ApplicationAssociatorStack', // Add your existing stack name here ... ``` This will result in the existing stack deploying with the previous name, and the id in CDK will reflect this same stack name. Related links: * Previous PR: #23823 * Previous GitHub issue: #23861 * Original reason that introduced `stackId`: #22508 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 25, 2023

aws-cdk-automation requested a review from a team January 25, 2023 00:49

aws-cdk-automation previously requested changes Jan 25, 2023

View reviewed changes

Fix trailing space and add integ test

c002d55

Merge branch 'main' into l2_fixes

ac84048

jungle-amazon marked this pull request as ready for review January 27, 2023 01:08

rix0rrr previously requested changes Feb 1, 2023

View reviewed changes

jungle-amazon changed the title ~~fix(servicecatalogappregistry): fix L2 ApplicationAssociator construct defects~~ fix(servicecatalogappregistry): conflicting stack names when multiple apps are deployed to same account and region Feb 2, 2023

Keep ApplicationAssociatorStack as default stack id, but use app name…

bf41d5a

… or id for stack name

Merge branch 'main' into l2_fixes

3b8514c

rix0rrr previously requested changes Feb 7, 2023

View reviewed changes

Deprecate stackID and recommend stackName use in TargetApplication …

aa800e5

…options

jungle-amazon and others added 2 commits February 7, 2023 12:57

Removed the extra text from Deprecated tag

d39f9bb

Merge branch 'main' into l2_fixes

5e7f04b

rix0rrr approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into l2_fixes

e6fd324

mergify bot merged commit 420b5ff into aws:main Feb 8, 2023

jungle-amazon mentioned this pull request Feb 14, 2023

fix(servicecatalogappregistry): Allow user to control stack id via stack name for Application stack #24171

Merged

jungle-amazon deleted the l2_fixes branch February 20, 2023 19:18

jungle-amazon restored the l2_fixes branch February 20, 2023 19:18

jungle-amazon deleted the l2_fixes branch February 20, 2023 19:18

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

WontonSam mentioned this pull request Oct 25, 2024

[Snyk] Upgrade @aws-cdk/assert from 1.128.0 to 2.68.0 WontonSam/apollo-federation-subgraph-compatibility#621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

jungle-amazon commented Jan 25, 2023 •

edited

Loading

gitpod-io bot commented Jan 25, 2023

aws-cdk-automation left a comment

rix0rrr left a comment

rix0rrr Feb 1, 2023

jungle-amazon Feb 2, 2023

rix0rrr Feb 7, 2023

jungle-amazon Feb 7, 2023

rix0rrr Feb 7, 2023

jungle-amazon Feb 7, 2023

rix0rrr Feb 7, 2023

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

mergify bot commented Feb 8, 2023

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

Conversation

jungle-amazon commented Jan 25, 2023 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 25, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Feb 1, 2023

Choose a reason for hiding this comment

jungle-amazon Feb 2, 2023

Choose a reason for hiding this comment

rix0rrr Feb 7, 2023

Choose a reason for hiding this comment

jungle-amazon Feb 7, 2023

Choose a reason for hiding this comment

rix0rrr Feb 7, 2023

Choose a reason for hiding this comment

jungle-amazon Feb 7, 2023

Choose a reason for hiding this comment

rix0rrr Feb 7, 2023

Choose a reason for hiding this comment

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 8, 2023

jungle-amazon commented Jan 25, 2023 •

edited

Loading